-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: allow circular references in config #17752
Conversation
Hi @fasttime!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I expect the latest value to be prioritized and overridden correctly when merging, so the new behavior looks fine. If the new value isn't in the expected format the program should automatically throw errors.
I believe we don't need to solve this edge case at the moment, we can look at it later if there is such a requirement from any user. |
I think we should revert it to the current ESLint behavior. Merging is complicated enough, and I don't want to risk introducing breaking changes.
I agree with @snitin315, I don't think we need to worry about this edge case. |
Thanks! I've reverted to the current behavior. Note that some unit tests will now assert seemingly unwanted results, but if you prefer to avoid unintentional breaking changes, I think we should keep those tests as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just a couple of comments about the behavior.
(Please feel free to let me know if we are already doing what I'm asking about and I just missed it.)
assert.notStrictEqual(result, first); | ||
}); | ||
|
||
it("merges an object in a property with a string", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is string a special case? It seems odd that the behavior is different between numbers and strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the current behavior. It's because of the destructuring here:
eslint/lib/config/flat-config-schema.js
Lines 86 to 89 in 5de9637
const result = { | |
...first, | |
...second | |
}; |
A primitive like "qux"
is converted into an object during destructuring i.e. Object("qux")
, which is similar to { "q": 0, "u": 1, "x": 2 }
.
A number like 42
will be also converted into an object when destructured, but since Object(42)
has no enumerable properties, the effect won't be visible, and it will behave like an empty object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Thanks for adding tests to make this clear. 👍
@@ -100,22 +100,28 @@ describe("merge", () => { | |||
assert.notStrictEqual(result, second); | |||
}); | |||
|
|||
it("overwrites a value in the first object with null in the second one", () => { | |||
it("throws an error if a value in a property is overriden with null", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this is the current behavior, but this is happening because of the call to Object.keys(second)
on this line:
eslint/lib/config/flat-config-schema.js
Line 91 in 5de9637
for (const key of Object.keys(second)) { |
The problem is that Object.keys
throws an error if called with an argument of null
or undefined
. Here, the argument (second
) cannot be undefined
, because it is assigned a default value in the declaration, but it can be null
.
If we only want to fix this test case, and the previous case where a string is being destructured into an object, then we could make sure that second
is an object before calling deepMerge
(and if it is a primitive, replace it with an empty object):
- const secondValue = second[key];
+ let secondValue = second[key];
if (isNonNullObject(firstValue)) {
+ if (!isNonNullObject(secondValue)) {
+ secondValue = EMPTY_OBJECT;
+ }
This would avoid the TypeError
when the second value is null
, and treat "qux"
as an empty object, rather than using its characters as properties, and it would keep the current behavior unchanged in all the other test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per #17820 - currently the ecosystem relies on the override behaviour - at least for null
.
So crashing out here will impact migration efforts and will mean that we have to redesign parser options to account for this change.
For reference - typescript eslint handles parserOptions.project = null
to mean "not set" (I.e. Type aware linting is turned off).
The current behaviour is that a user can turn on type-aware linting in their base config (eg parserOptions.project = ["some TS project path"]
) then pick specific parts of their codebase for which to disable type-aware linting using eslintrc overrides and setting parserOptions.project = null
.
For eslintrc merging - the null
clobbers the value - enabling this behaviour.
There's no other way to clobber or erase the value in an overrides because an empty array just merges in and is thus a no-op.
As I said - removing this behaviour would set the ecosystem back unless an alternative is provided. Being able to "clear" a property in an override is quite a useful feature for complex configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fasttime ah okay, thanks for explaining.
@bradzacher for the purposes of this PR, we don't want to change the default behavior. We'll discuss on your issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like @mdjermanovic to take a look before merging.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
I also think it should work this way. We can fix this in a PR for #17820. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
* main: (25 commits) test: ensure that CLI tests run with FlatESLint (eslint#17884) fix!: Behavior of CLI when no arguments are passed (eslint#17644) docs: Update README Revert "feat!: Remove CodePath#currentSegments" (eslint#17890) feat!: Update shouldUseFlatConfig and CLI so flat config is default (eslint#17748) feat!: Remove CodePath#currentSegments (eslint#17756) chore: update dependency markdownlint-cli to ^0.38.0 (eslint#17865) feat!: deprecate no-new-symbol, recommend no-new-native-nonconstructor (eslint#17710) feat!: check for parsing errors in suggestion fixes (eslint#16639) feat!: assert suggestion messages are unique in rule testers (eslint#17532) feat!: `no-invalid-regexp` make allowConstructorFlags case-sensitive (eslint#17533) fix!: no-sequences rule schema correction (eslint#17878) feat!: Update `eslint:recommended` configuration (eslint#17716) feat!: drop support for string configurations in flat config array (eslint#17717) feat!: Remove `SourceCode#getComments()` (eslint#17715) feat!: Remove deprecated context methods (eslint#17698) feat!: Swap FlatESLint-ESLint, FlatRuleTester-RuleTester in API (eslint#17823) feat!: remove formatters except html, json(-with-metadata), and stylish (eslint#17531) feat!: Require Node.js `^18.18.0 || ^20.9.0 || >=21.1.0` (eslint#17725) fix: allow circular references in config (eslint#17752) ...
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Fixes #17665
What changes did you make? (Give an overview)
This PR fixes an issue that would cause a stack overflow when configuration
settings
orparserOptions
contain a circular reference.The fix uses a two-dimensional map in the
deepMerge
function to keep track of the results previously created when merging pairs of arguments. If the same pair of arguments is encountered later on while merging another property, the previous result will be returned immediately. This avoids infinite recursion.Is there anything you'd like reviewers to focus on?
I have questions about two important implementation details. Would like reviewers to confirm if my approach is okay or how to change it.
Question 1
What is the expected result when merging an object with a primitive or a function? E.g.
To me, none of the above examples seems to behave as intended in the current implementation. In my unit tests, I'm assuming that when two values cannot be merged because they are not both objects, the expectation is to just pick the second value if it's not undefined. That's how Lodash's
merge
works, for example, except that it also merges arrays while we don't.I'm leaving this as a question for reviewers if we want to keep this behavior like that, revert it to how it was before, or change it in another way.
Question 2
There is still some potential to produce a stack overflow even without a circular reference, for example if a property evaluates to a different object on every access. Here's an example:
I think this is a constructed case we don't need to care about, but if we decide to do so and handle this situation in a way or another, then we should also have a look at
normalizeRuleOptions
:eslint/lib/config/flat-config-schema.js
Line 132 in 5de9637
The
structuredClone
algorithm we use there will similarly fail with an error when non-circular properties exceed the depth that fits inside the stack - but again, I don't expect this situation to happen for a reasonable config.